Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix multus config file generation to avoid self-delegation #1137

Closed
wants to merge 1 commit into from
Closed

fix multus config file generation to avoid self-delegation #1137

wants to merge 1 commit into from

Conversation

thomasferrandiz
Copy link

Add a check to make sure that we don't use an existing multus configuration file. This avoids creating a situation where multus delegates to itself and breaks pod networking by trying multiple times to create the same intefarce for a pod.

This should fix: #1130

@thomasferrandiz thomasferrandiz mentioned this pull request Aug 11, 2023
Closed
s1061123
s1061123 requested changes Aug 11, 2023
View reviewed changes
cmd/thin_entrypoint/main.go Outdated
Comment on lines 301 to 302
// where multus delegates to itself and breaks pod networking
multusRegexp, err := regexp.Compile("multus")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently thin_entrypoint creates 00-multus.conf or 00-multus.conflist. So could you please use "00-multus.conf{,list}" to match the multus generated file?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, thanks for the input.

@coveralls
Copy link

Coverage Status

coverage: 64.099% (+0.3%) from 63.783% when pulling ea7f19b on thomasferrandiz:delegateAdd-bug into d5883bd on k8snetworkplumbingwg:master.

@thomasferrandiz
fix multus config file generation to avoid self-delegation
4b973ea 
Add a check to make sure that we don't use an existing multus configuration file.
This avoids creating a situation where multus delegates to itself and breaks
pod networking by trying multiple times to create the same intefarce for a pod.

Signed-off-by: Thomas Ferrandiz <[email protected]>
@s1061123
Copy link
Member

s1061123 commented Aug 16, 2023 •
edited
Loading

I locally tested simpe go code from your PR code and found that your code still choose 00-multus.conf, hence your code does not fix this issue. Please check your code?

package main

import (
        "fmt"
        "path"
        "regexp"
)

func main() {

        filePathLists := []string{
                "/host/etc/cni/net.d/00-multus.conf",
                "/host/etc/cni/net.d/10-flannel.conflist",
        }

        multusRegexp, err := regexp.Compile("00-multus.conf{,list}")
        if err != nil {
                fmt.Printf("regexp compilation failed: %v", err)
                return
        }
        masterConfigPath := ""

        for _, filename := range filePathLists {
                if !multusRegexp.MatchString(path.Base(filename)) {
                        masterConfigPath = filename
                        break
                }
        }
        fmt.Printf("masterConfigPath: %s\n", masterConfigPath)
        return
}

$ go run test.go
masterConfigPath: /host/etc/cni/net.d/00-multus.conf

@s1061123
Copy link
Member

File another PR for this issue: #1142 so let me close this PR.

@s1061123 s1061123 closed this Aug 16, 2023
@thomasferrandiz
Copy link
Author

The original PR without the modification you requested actually worked.

After testing, the correct regexp would be regexp.Compile("00-multus.(conf|conflist)").

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s1061123 s1061123 s1061123 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Infinite recursion when setting up Multus (delegates to itself)
3 participants
@thomasferrandiz @coveralls @s1061123